Stop discarding a framework's JSON args and its measured accuracy - #1332

The quote-preserving tokenizer closed the JSON corruption only for strings
it accepts. `_tokenize_for_removal` still fell back to a POSIX
`shlex.split` on the ones it declines, and that fallback applies to the
WHOLE string: one whitespace-bearing operand anywhere -- say a
`--tool-call-parser 'my parser'` sharing the args with atom's
`--online_quant_config` -- stripped the inner quotes off the JSON blob
beside it and produced the same `{global_quant_config:ptpc_fp8,...}` the
server rejected. `_repair_unquoted_json` cannot re-quote the `*expert*`
wildcards, so it reached the server broken exactly as before.

It also made the two sides of the comparison disagree: args tokenized
POSIX while the removal spec tokenized quote-preserving, so the flag was
not matched and not removed either.

Drop the fallback. A string the tokenizer declines carries a token the
unquoted `EXTRA_*_ARGS` transport cannot represent at all, so it is
already unlaunchable, and splitting it anyway corrupts every JSON blob
sharing it to strip a flag off a string that was going to fail regardless.
Returning it untouched is what every other caller of
`tokenize_server_args_preserving_json` already does -- dedup and the
coordinator helpers return the input, prelude and warm replay raise -- and
it matches this function's own documented "left untouched rather than
guessed" contract. The only cost is an unstripped
`--no-enable-prefix-caching`, which is visible, against a corrupted JSON
value that is not.

Three cases added; all three fail on the parent commit.

The accuracy gate now grades every variant that has a reference, but
whether an eval actually ran is resolved from the materialized RUN_EVAL
contract -- base config, reference envs, the variant's own extra_envs,
the process env -- none of which the explore executor controls. A stale
config or a proposal carrying RUN_EVAL=false therefore leaves no score
on disk, and the gate fails the variant closed as accuracy_unavailable:
a broken eval path reads as "no gains found" rather than as a broken
eval path.

Force RUN_EVAL=true under exactly the gate's own entry condition, so
being graded and being measured cannot come apart. It lands on the round
the gate reads -- the warmup under warm-decision, where the decision
round stays throughput-only and parse_eval_results falls back to the
warmup's score, and the decision round itself otherwise -- and is
applied after the variant's envs so a proposal cannot switch off the
eval its own KEEP depends on. --no-eval still wins, since it leaves
baseline_accuracy at 0 and skips the gate outright.

The comment claiming the lifecycle already evaluates every warmup_round
unconditionally was wrong and is corrected: sweep, rebench, the baseline
measure round and the explore decision rounds all set RUN_EVAL false.

Dropping the hardcoded conc_sweep_failed -> CLOSE short-circuit sent every
failed closeout scan into the R1 reloop decision, which weighs the macro-cycle
cap, direction saturation, the no-gain streak and remaining budget -- but not
whether OPTIMIZE has already reported both arms plateaued. main's walkthrough
reaches CLOSE on a run where both arms are dry, and it reached it through that
short-circuit rather than through convergence, so removing the short-circuit
made the run reloop into a phase that had just said it was out of moves. With
the no-gain streak needing 3 cycles, a dry run burned two extra macro-cycles
before winding down.

conc_sweep_failed is now terminal exactly when this cycle's OPTIMIZE exit
recorded both arms plateaued. The incident case is untouched: that run was
mid-cycle with gain still landing and died because corrupted JSON killed the
server, not because the arms were dry.

The recorded exit is read rather than the plateau predicates re-derived --
KERNEL_AGENT and SWEEP move the state those predicates read, so re-running
them at SWEEP exit answers a different question than OPTIMIZE answered. Only
the plateau flavour counts: optimize_no_more_leverage also covers skip_to_sweep
and an LLM escalation, which ask for SWEEP sooner rather than reporting dry
arms.

Five cases covered: plateau closes, no recorded exit / skip_to_sweep /
llm_escalation still reloop, and a plateau stamped with another cycle does not
close this one.

Declining the whole removal when any token was undeliverable meant one quoted
operand anywhere in the string silenced every removal on it. This is a launch
sink -- _workload_envs writes the result straight into EXTRA_*_ARGS, and
compose_server_args always ends in strip_benchmark_harness_flags -- so the flag
that should have been dropped was served, benchmarked, and written to the
ledger, with the gain attributed to a configuration nobody can serve. Nothing
was logged, while the fail-closed path next door in _workload_envs warns.

The trigger was also much wider than the whitespace-bearing case it was written
for: the guard rejects any edge quote, so --tool-call-parser 'hermes' -- one
token, perfectly launchable -- disabled removals too.

shlex in non-POSIX mode keeps quote bytes, so a space-join of its output
reproduces the input byte for byte, including the JSON whose inner quotes a
POSIX split drops. Removal can therefore always proceed; only the diagnostic
changes. Undeliverable tokens are passed through verbatim and logged, since
they arrive malformed and skipping the removal does not make them launchable.

Removing a flag now consumes every fragment of its value up to the next flag.
A JSON value containing whitespace survives shlex as several tokens, and
consuming only the first left the rest behind as bare argv words.

Also drops the claim that every other caller of
tokenize_server_args_preserving_json fails closed on None. Only coordinator.py
logs; coordinator_helpers and three call sites here return the input unchanged,
which is the same silence this commit removes.

The gate and the RUN_EVAL injection were two separate conditions: the gate read
`scriptable or baseline_accuracy > 0`, the injection added `and not
eval_disabled`. A session that opted out of eval while a measured baseline was
still on the state -- or any scriptable framework, which gates without
consulting a baseline at all -- was therefore graded on a score the round was
never going to produce, REVERTing every variant as accuracy_unavailable. That
is the exact failure the injection was added to remove.

Both now read accuracy_gate_applies() in _accuracy_gate, so they cannot
diverge again, and eval_disabled skips the gate rather than only the injection.
Extracting the predicate also gives it real coverage: the previous test only
asserted the deleted flag catalogue had not come back, which said nothing about
what replaced it.

SharedState.baseline_accuracy now wins over params. accuracy_baseline is
offered to the LLM in the action schema while every in-tree writer only copies
SharedState's value, so a params figure that disagrees is a hallucination, not a
second opinion -- and it is logged when ignored. This mattered less when the
gate ran on a handful of high-risk variants; with the gate reading for every
variant that has a reference, one bad number REVERTs the whole grid or marks all
of it accuracy_drop. Params is still the only source when nothing measured a
baseline, which keeps the documented external-invocation behaviour.

Also drops the module docstring's "high-risk variants too", which described the
predicate deleted earlier in this branch.

The convergence branch returned a hardcoded global_converged, discarding
exit_reason. A run whose conc_sweep failed and whose reloop was then blocked by
R7 convergence reported a clean convergence, and reset_per_cycle_plateau_state
clears last_conc_sweep, so the failure could not be recovered from state
afterwards either. Convergence explains why no further cycle opened, not how
this one ended, so the failure now displaces the label while reloop_blocked
keeps the convergence in the evidence.

The report text for conc_sweep_failed claimed no budget remained. Budget was
only ever one of the reasons a wind-down happens, and after the plateau check
earlier in this branch it is not even the most likely one, so it now points at
reloop_blocked instead of naming a cause it cannot know.

Coverage was insufficient_remaining only; the convergence path and the
unchanged clean-sweep label are now asserted too.

…args-and-unconditional-accuracy

The span consumed everything up to the next ``--``, so a removal ate any
single-dash option that followed it: stripping --no-enable-prefix-caching off
'--no-enable-prefix-caching -tp 8 --max-num-seqs 64' also deleted -tp 8. That
trades the previous bug for a worse one on the same unconditional launch path --
deleting flags nobody asked to remove instead of failing to remove one.

The span now ends at any option name, long or short, with a leading dash
followed by a digit still read as a negative value. It may only end once the
JSON braces are balanced, so a value fragmented by whitespace keeps its
fragments; the brace scan carries its string state across tokens, because a
fragment ending mid-string left the next fragment's closing quote read as an
opening one, which hid the brace that balanced the blob.

Comparing a pair spec against the whole span also made ``--foo bar`` a no-op on
'--foo bar baz'. A pair spec names one operand, so it matches the first value
token too.

_optimize_declared_no_leverage only matched optimize_no_more_leverage. With
KERNEL disabled the same exit is recorded as no_kernel_skipped with the real
reason under evidence.passed_through_reason, so the guard was inert on exactly
the path where a reloop has no KERNEL arm left to switch to.

The unparseable-input branch no longer logs the whole args string, which can
carry credentials, and the docstring no longer claims such inputs are left
untouched -- they are handled best-effort, because this is the sink every
compose reaches.

The baseline reconciliation moves into _accuracy_gate so integrate_patch's KEEP
gate gets it too. _framework_run_eval_envs keeps reading params: it only decides
whether to force the eval on, so a wrong figure there costs a spare eval rather
than a verdict.

report.py no longer promises reloop_blocked, which the producer's own evidence
does not carry.

It was called the single source of truth for "did eval run this round", which
reads as authority over whether eval should run -- and a graded round overrides
it on purpose. It reports what the subprocess was told; the session-level
--no-eval is what means "do not evaluate", and that skips the grading too.

…peating

Three problems in the removal sink, all on the path every compose reaches.

A pair spec matched a flag's first operand and then deleted exactly two
tokens, so `remove_server_args('--cuda-graph-bs 1 2 4 --tp 8',
['--cuda-graph-bs 1'])` returned '2 4 --tp 8'. The leftovers are bare argv
words: validate_server_args_shell_safe rejects them outright and aborts the
launch, and the paths that skip that validator hand them to the server as
positional arguments. A flag's operand list now goes as a unit, which is what
the span logic two lines above was written for.

The undeliverable-token warning fired on every compose of every variant of
every cycle, so one legitimate quoted operand printed the same multi-line
block hundreds of times per session -- and it interpolated the removal specs,
which are author-supplied through the same path as the args string the
docstring pointedly refuses to echo. It now reports once per distinct option
set per process, and names the option the undeliverable token belongs to
rather than any value.

The tokenizer docstring claimed a byte-for-byte round trip, which is not true
(JSON blobs are recompacted, separators collapse, the untokenizable fallback
keeps no quoting at all) and is not what makes the removal safe. Restated on
the property that does hold: each token keeps its own bytes, so a retained
neighbour is never rewritten.

Removal tests now compare whole strings. `'--flag' not in out` also passes on
'' and on output whose surviving flags lost their operands, which is how both
silent deletions reached review with the suite green.

accuracy_gate_applies only honoured --no-eval, so a session whose base YAML or
reference_envs deliberately set RUN_EVAL=false (no lm-eval in the benchmark
venv) while carrying a positive baseline from an earlier phase still had
RUN_EVAL='true' injected per variant. The eval cannot run, parse_eval_results
finds nothing, and every variant REVERTs as accuracy_unavailable -- the exact
failure the injection was added to prevent.

It now reads materialized_run_eval_disabled, which this module already held for
the other two arms. Read against the config explore materializes BEFORE variant
envs fold in, so the two RUN_EVAL=false spellings stay separable: the
environment saying it cannot evaluate opts the gate out, while a variant
switching off the eval its own KEEP is gated on is still overridden.

reconcile_baseline_accuracy guarded float() on the proposed value but not on the
state value, so a state.json with a non-numeric baseline_accuracy raised
ValueError out of the whole KEEP decision. Both sides share one coercion now
and a corrupt state value degrades to "no measured baseline", as the code this
replaced did. Its docstring also asserted that all in-tree writers copy
SharedState; it now states the rule it actually implements -- SharedState is the
only supported channel -- and the log line names the baseline writeback as the
way a genuinely re-measured reference takes effect.

Per variant, a missing accuracy verdict is indistinguishable from a broken eval
path, so each stays fail-closed. Across a round it is not: the round now counts
verdicts against misses and reports eval_infrastructure_suspected when not one
gated variant scored, so a run ending at zero KEEP says whether that was a
verdict on the knobs or on the harness.

Also documents why the forced eval needs no separate deadline allowance: the
double-run baseline reports the WARMUP round's wall-clock as
subprocess_runtime_sec, and that round is the one that ran RUN_EVAL=true, so
both sides of the fit estimate already include one eval.

_optimize_declared_no_leverage filtered phase_history by the cycle stamp alone.
make_history_row documents cycle=0 both for the first macro-cycle and for a
resume from a pre-cyclic session, so at cycle 0 the stamp cannot separate them:
a plateau exit restored from an earlier session matched and closed the run on a
plateau this cycle never reported, forfeiting the budget the rest of this branch
exists to keep. Reachable whenever the current cycle has no OPTIMIZE exit of its
own to find first. The scan now stops at the previous cycle boundary -- a SWEEP
exit ends a macro-cycle, the PRELUDE handoff opens the first one.

Not short-circuiting on conc_sweep_failed is what kept a 16h run from forfeiting
9.8h to a closeout scan, but the failure in that incident was deterministic: a
corrupted --online_quant_config killed every conc_sweep launch. With nothing
carried across cycles the run re-ran FRAMEWORK_AGENT -> KERNEL -> SWEEP ->
conc_sweep and died identically each cycle until the budget or max_cycles ran
out. reset_per_cycle_plateau_state clears last_conc_sweep, so the durable record
is the reloop row's own sweep_exit_reason, already stamped by compute_next_phase.
One reloop is the retry that incident argued for; a second identical failure has
shown that retrying reproduces it, and closes with
reloop_blocked=conc_sweep_failed_repeated.

remove_server_args modelled its input with shlex and then reassembled with
" ".join. Neither matches the sink: Magpie expands EXTRA_*_ARGS through an
unquoted shell wrapper, which word-splits on whitespace and performs no quote
removal. Every disagreement between the model and the sink surfaced as a
removal that deleted or rewrote the wrong bytes, and three review rounds found
three separate instances:

  - A span that tracked JSON brace depth could enter a blob that never
    balanced ('--foo a}') and run to the end of the string, so removing one
    flag silently deleted the whole tail of the configuration and the server
    booted without it. Worse than the bug this branch set out to fix.
  - Quoted operands were compared byte-for-byte, so remove_args ['--foo bar']
    was a no-op against '--foo "bar"' -- a reverse regression against main, in
    the spelling operators and LLMs write most often.
  - " ".join collapsed whitespace inside retained JSON string values, so
    {"chat_template": "a  b"} reached the server as "a b" -- the same class of
    silent value modification as the original incident.

Split on whitespace while keeping the separator pieces instead. Reassembly is
then byte-exact for everything retained, comparison runs on a quote-normalized
logical value, the span is simply "up to the next option name" with no depth
state to run off the end of, and there is no untokenizable path left because
nothing remains for a quote to unbalance. All three cases above are fixed by
the model change rather than by three more special cases.

A pair spec that names one operand of a multi-operand flag still removes the
whole operand list -- splitting it leaves bare argv words -- but now logs a
WARNING, since the gain or regression then belongs to a different knob than
the one the spec named.

Tests: enumerate the token shapes that reach this sink and assert invariants
over the product (neighbours byte-identical, removal idempotent, no orphaned
operands) at head/middle/tail positions with every removal spelling. The
hand-written matrix kept missing the next case because it was written from
memory of the previous bug; this found two on the first run.

…it needs to

Removed rather than repaired, because both guards read state that cannot carry
the meaning they depend on:

  - _optimize_declared_no_leverage gated on evidence.plateau from the recorded
    OPTIMIZE exit. But exit_normal_optimize stamps plateau: True on the routine
    "source_dry and config_dry" end of a cycle -- that is what an ordinary
    cycle looks like, not a statement that the arms are spent, since a new
    cycle re-seeds both. Gating on it turned conc_sweep_failed terminal on a
    normal cycle and forfeited the remaining budget, reproducing the very
    incident this branch exists to eliminate from a second branch.
  - _conc_sweep_failure_already_relooped divined "did a previous cycle already
    retry" from phase_history. That log is capped at _PHASE_HISTORY_CAP (100)
    rows, so a long run evicts the reloop row and the cap silently stops
    applying; and it is restored across sessions, so a resumed run finds the
    row immediately and closes on its FIRST failure. Wrong in both directions,
    in the two scenarios the cap was added for.

conc_sweep is a closeout concurrency scan, not the optimization, so its failure
says nothing about whether the remaining budget could still find gain.
conc_sweep_failed now never short-circuits the reloop.

Capping deterministic conc_sweep retries is still worth doing, but it needs a
durable SharedState counter (incremented on failure, reset on success) rather
than a predicate reconstructed from a capped, resume-restored log. Tracked
separately; deliberately not in this change, which is about not forfeiting
budget.

- explore.py: don't force RUN_EVAL=true for scriptable frameworks. Their gate
  compares the bench script's own quality figure against a fixed 1.0
  reference, so the forced eval pass per variant fed nothing.
- explore.py: state at the call site that materialized_run_eval_disabled stays
  fail-closed in this direction -- an unreadable config reports "not disabled",
  which leaves the accuracy gate ON.
- integrate_patch.py: drop `or None` after reconcile_baseline_accuracy, which
  converted a legitimate 0.0 baseline into None.
- _grid_server_args.py: single definition of _is_flag_token, imported from
  _canonical_fingerprint; the two copies had already drifted once.
- _grid_server_args.py: the undeliverable-token warning cache now evicts its
  oldest entry when full instead of clearing wholesale, which re-armed every
  option already reported, and keys on the payload size as well as the option
  names.

remove_server_args computed the operand span for the space-separated spelling
only. On --cuda-graph-bs=1 2 4 it dropped the one --flag=value word and left
2 and 4 behind as bare argv words -- the exact outcome the operands-as-a-unit
rule exists to prevent, reached through the spelling that rule never looked at.
validate_server_args_shell_safe rejects that string outright, and the paths
that skip it hand the leftovers to the server as positionals.

The shape is malformed at the source (argparse reads the extras as positionals
rather than as the flag's list) and the sink validator already refuses it
either way, so nothing launchable regressed. But the invariant this function
documents is unconditional, and it was not.

The span is now computed once, before the two branches that consume it, and the
equals branch drops through the same end index. With no trailing operand -- the
overwhelmingly common case -- end is i+1 and the behaviour is byte-identical.
The attribution warning moves to a helper so both branches emit one message
rather than two copies drifting apart.

Mechanically checked against a clean main worktree over 520 args/spec
combinations: 0 cases where main produced a launchable string and this does
not, 200 where main's output was unlaunchable and this one is.

Replacing the tokenizer with a whitespace split left _span_end deciding where a
value ends by asking _is_flag_token alone, so a JSON string value carrying a
dash-prefixed word ended the span inside itself:

    remove_server_args('--tp 8 --tmpl {"t":"Answer --now please"} --max-num-seqs 64',
                       ['--tmpl'])
      main -> '--tp 8 --max-num-seqs 64'
      here -> '--tp 8 --now please"} --max-num-seqs 64'

The leftover is not argv, so validate_server_args_shell_safe aborts the launch
in _finalize_framework_server_args -- a string main removed cleanly. Over 414
(args, spec) combinations against a clean main worktree this was 12 cases of
"main launchable, this branch not", against the 0 the PR body reports; the
mechanical sweep missed them because not one of its token shapes carried a
fragment starting with a dash.

The same blind spot let a fragment be removed as if it were a flag. A denylist
name inside a string value is matched by strip_benchmark_harness_flags, which
rides on every compose, and cut the blob in half with no spec asking for it.

The span may now run past an option name while the JSON scan is unbalanced, and
only as far as the word that balances it. Nothing balancing it keeps the plain
scan, so an operand carrying a stray } still cannot run the span off the end of
the string -- the bug an earlier revision traded for this one. The removal loop,
the spec parser and the undeliverable-token report all read the same
_words_inside_json, so the two sides of the comparison cannot disagree about
what is a flag, and the warning cannot name a byte of somebody's value as an
option. That set clamps a negative depth to closed: carrying it forward would
mark every word after a stray } as nested and silently disable every removal
after it.

Tests: the shape matrix gains {"t":"a --b c"}, so the four mechanical invariants
run over it too, plus a class covering the span, the sink verdict, the
unremovable lookalike, a neighbour removal leaving the value byte-exact, and
both directions of the unbalanced fallback. Eight fail on the parent commit.
Failure sets over the five suites reaching this sink are identical to the parent
(35 pre-existing, zero new); ruff check and ruff format --check clean.

Co-authored-by: Cursor <cursoragent@cursor.com>

Four rounds of review grew a two-line brief into 1820 insertions across 14
files. This reverts everything that was not one of the two fixes and reduces
what remains to 149 insertions across 7.

JSON args. remove_server_args now compacts the JSON values before tokenizing
and splits with posix=False. Compacting leaves each JSON value as one
whitespace-free word, so the non-POSIX split keeps it whole and keeps its own
double quotes -- the quotes the POSIX split ate, turning {"a":"b"} into {a:b}
and killing every conc_sweep launch in json.loads. The JSON arrives with no
shell wrapper because compact_json_server_args strips it upstream, which is
why the POSIX split could reach the quotes at all. _repair_unquoted_json was
supposed to recover this and could not: its bareword pattern has to guess where
the quotes went, and atom's exclude_layer wildcards (*.mlp.gate, *expert*) fall
outside that guess. Removing the guess is enough; the round-trip is now
lossless, so nothing has to be repaired afterwards.

Gone with it: the 504-line rewrite of this module's tokenization model. Its
hand-written "split keeping separators, track bracket depth, classify
option/value" parser cost four consecutive silent-deletion bugs in
remove_server_args, one per review round, and ~617 lines of property tests to
be trusted at all. It was solving a problem that compacting removes.

Accuracy gate. is_high_accuracy_risk, _HIGH_RISK_CLI_PATTERNS and
_HIGH_RISK_ENV_KEYS are deleted and EXPLORE gates every variant that has a
reference. The eval runs on every warmup round regardless, so the score is
already on disk and the catalogue's only effect was discarding it -- for atom
it discarded everything, because --kv_cache_dtype never matched
--kv-cache-dtype and --online_quant_config was never enrolled.

Gone with it: the eval_disabled / run_eval_disabled / materialized veto
predictor, the forced RUN_EVAL injection, the round-level accuracy_gate
summary, and reconcile_baseline_accuracy. None of that was asked for, and the
veto is what turned the scriptable image-quality gate from fail-closed to
fail-open.

Also reverted, unrelated to either fix: machine_state.py and report.py (the
conc_sweep_failed reloop, which needs a durable SharedState retry counter
rather than a predicate read off a capped phase log -- separate change),
integrate_patch.py, and the _SPACE_VALUE_FLAGS deletion in _grid_runner.py.

Tests. test_json_server_args_roundtrip.py is deleted. Two cases replace it in
test_grid_runner_helpers_coverage_unit.py, and one in test_explore_executor.py.
All three fail on the code before this change: the first with the same
"Expecting property name enclosed in double quotes" the server reported, the
third with KEEP where the gate should now REVERT.

Co-authored-by: Cursor <cursoragent@cursor.com>

Gating every variant that carries a reference widened the blast radius of a
wrong ``accuracy_baseline``. It is offered to the LLM in the explore action
schema, and it outranked ``SharedState.baseline_accuracy``, so a proposed figure
that disagrees now fails a whole grid where it used to reach only the variants a
flag catalogue called risky. Every in-tree writer -- framework.py, proposals.py,
integrate_patch.py -- copies the state value, so inverting the precedence
changes nothing for any real flow and closes the hallucination path. params stay
as the fallback for an external invocation that carries no state.

Also dropped a ``assert not hasattr(...)`` test that only restated the deletion;
the behaviour is covered by the explore case that gates an uncatalogued knob.

Also corrected two comments that claimed the eval "runs on every warmup round
regardless". It runs whenever RUN_EVAL is on, which is the default; what
actually keeps serving ungated when a session opts out is that
``baseline_accuracy`` stays 0, as shared_state.py already documents.

Co-authored-by: Cursor <cursoragent@cursor.com>